[WIP] Initial multiple queues support#865
[WIP] Initial multiple queues support#865mikooomich wants to merge 13 commits intoFoedusProgramme:betafrom
Conversation
| customExtras.putInt("index", index) | ||
| }, Bundle.EMPTY | ||
| ).get().extras.run { | ||
| if (containsKey("allQueues")) { |
There was a problem hiding this comment.
why bother with containsKey if you throw anyway, you can just let the !! do its job
| /** | ||
| * Deletes a queue. | ||
| * | ||
| * When deleting the active queue, |
| if (index == masterQueues.lastIndex) { | ||
| masterQueues.removeAt(index) | ||
| if (index <= 0) { | ||
| player.pauseAllPlayersAndStopSelf() // TODO: correct way to stop playback |
| val endedWorkaroundPlayer | ||
| get() = mediaSession?.player as EndedWorkaroundPlayer? | ||
| private var controller: MediaBrowser? = null | ||
| val qb: QueueBoard = QueueBoard(this) |
There was a problem hiding this comment.
maybe should be inited in onCreate
| } | ||
|
|
||
| // TODO: shuffle and repeat mode | ||
| fun MediaController.playQueue( |
There was a problem hiding this comment.
you are doing this at the wrong level, Android AUto for example will just not call your SERVICE_QB_ENQUEUE, it will keep doing setMediaItems(). Instead a player wrapper should give old queue to queueboard before executing setMediaItems()
| /** | ||
| * Retrieve a song given a song ID. Returns null if no song is found | ||
| */ | ||
| fun findSong(mediaId: String): MediaItem? { |
There was a problem hiding this comment.
For future uses. Should i remove it for the time being?
There was a problem hiding this comment.
having smth for future use is ok if you know for what use, but atleast for me its not really clear what its useful for
|
|
||
| val current = (instance?.currentMediaItemIndex ?: 0) | ||
| if (current > playlistAdapter.playlist.second.size) { | ||
| // hax to workaround ui loading itself 4 times |
| controller?.setMediaItems(albums.shuffled().flatMap { it.songList }) | ||
| controller?.prepare() | ||
| controller?.play() | ||
| controller?.playQueue( |
There was a problem hiding this comment.
cf, this ui code shouldnt be changed. isOriginal is always true after setMediaItems(), it should only be considered changed after add/remove/move(/replace if id changed)
|
|
||
| if (startIndex == 0) { | ||
| val playerItemCount = plr.mediaItemCount | ||
| // player.player.replaceMediaItems seems to stop playback so we |
There was a problem hiding this comment.
replaceMediaItems probably does but replaceMediaItem without s does not, update current song with repalceMediaItem too
There was a problem hiding this comment.
The currently playing media item can be at any index, and it needs to become the first index.. I'm not sure how replaceMediaItem would work better than what I current have.
There was a problem hiding this comment.
I mean in addition to what you have, as replaceMediaItem updates metadata without interruptting playback
| return null | ||
| } | ||
|
|
||
| // I have no idea why this value gets reset to 0 by the end... but ig this works |
| <string name="actions_query_shuffle_specific">shuffle $item_name</string> | ||
| <string name="please_allow_to_delete">Press allow to delete files</string> | ||
| <string name="choose_sd">Choose %s</string> | ||
| <string name="settings_mq_preview">Enable multiple queues preview</string> |
There was a problem hiding this comment.
try to avoid mentioning preview, instead use strings that can be reused once feature is stable, to avoid double work for translators
02e20d1 to
4304106
Compare
* And so it begins: My attempt to implement one of OuterTune's most cursed features (code wise), but without programming crimes.
mq: wip queue loading ui mg: Guard behind flag mq: Queue expiry * LocalDateTime methods shouldn't need sdk 26. Figure it out later
| startPositionMs = startPositionMs | ||
| ) | ||
| queueBoard.commitQueue(mq) | ||
| return Futures.immediateVoidFuture() |
There was a problem hiding this comment.
commitQueue calls setCurrQueue, which then calls realSetMediaItems, so calling handleSetMediaItems setmediatems just leads to a duplicate call
There was a problem hiding this comment.
I thought we already etablished that queueboard should only own inactive queues and the title of the active queue. that is, in setMediaItems it should copy the old queue into quereboard, not the new one. because if you want to do it this way then you also need to override at least handleAddMediaItems, handleMoveMediaItems, handleReplaceMediaItems, handleRemoveMediaItems, handleSeek which is stupid amount of effort, while if qb doesn't own current queue 1. nothing related to resumption etc needs to change 2. you avoid all of that spagetthi.
|
Hello. It has been... a while. I don't remember if I made all the requested changes, so if i missed anything plz let me know. Known issues I will take care of... soon
Please help me figure out why the song recycler can scroll down but not up. Please please please please |
nift4
left a comment
There was a problem hiding this comment.
Sorry don't have time for nested scroll debugging for at least 2 weeks, all I can give you rn is this quick review pass
| startPositionMs = startPositionMs | ||
| ) | ||
| queueBoard.commitQueue(mq) | ||
| return Futures.immediateVoidFuture() |
There was a problem hiding this comment.
I thought we already etablished that queueboard should only own inactive queues and the title of the active queue. that is, in setMediaItems it should copy the old queue into quereboard, not the new one. because if you want to do it this way then you also need to override at least handleAddMediaItems, handleMoveMediaItems, handleReplaceMediaItems, handleRemoveMediaItems, handleSeek which is stupid amount of effort, while if qb doesn't own current queue 1. nothing related to resumption etc needs to change 2. you avoid all of that spagetthi.
| mediaItemIndex: Int, | ||
| isOriginal: Boolean | ||
| ) { | ||
| sendCustomCommand( |
There was a problem hiding this comment.
qb doesn't own the current queue. Yes, a full queue was added into qb via addqueue, but all that data (with the exception of the title) remains untouched and isnt actually used anywhere. All the old data is overwritten anyways with the latest from the player when setmediaitems is called again, so there is no spaghetti required.
I see how that commitQueue nonsense is redundant, so I'll change that. I'll null out the info in addQueue to prevent that info from creeping in in the future, and also call super.
Now (fe511a1) it works like this for when setmediaitems is called: handleSetMediaItems -> addqueue adds skeleton queue to qb, -> commitqueue facilitates the active/inactive swap within qb (without its own setmediaitems) -> super.handleSetMediaItems
And then for user initiated queue swaps: commitqueue facilitates the active/inactive swap within qb (with realSetMediaItems) -> super.handleSetMediaItems. You cant call setmediaitems again
| try { | ||
| mediaSession?.player?.setMediaItems( | ||
| items.mediaItems, items.startIndex, items.startPositionMs | ||
| val mq = endedWorkaroundPlayer?.queueBoard?.addQueue( |
There was a problem hiding this comment.
see above, I think this part shouldnt need to change at all and qb should only manage inactive queue
| } | ||
| } | ||
|
|
||
| SERVICE_QB_GET_QUEUE -> { |
There was a problem hiding this comment.
what is get for? can't we just have user call LOAD and then read it? or is there some UI where content of inactive is needed?
There was a problem hiding this comment.
GET returns the queue with shuffle indexes for ui purposes, while LOAD is intended to facilitate the loading of the queue into the player. I don't believe these should be the same command.
| } | ||
|
|
||
| /* | ||
| SERVICE_QB_ENQUEUE -> { |
| throw IllegalStateException("shuffleFactory was found orphaned") | ||
| if (isForPlayback && items.mediaItems.isNotEmpty()) { | ||
| endedWorkaroundPlayer?.nextShuffleOrder = factory.toFactory() | ||
| // endedWorkaroundPlayer?.nextShuffleOrder = factory.toFactory() |
|
|
||
| plr.exoPlayer.setShuffleOrder(seed.toFactory()(mq.startIndex, mq.getSize(), plr)) | ||
| } else { | ||
| Log.d(TAG, "Seamless is not supported. Loading songs in directly") |
There was a problem hiding this comment.
I'd prefer not. I mean, I'm willing to guard it behind a flag, but I wish to keep some debug logs around
| Log.d(TAG, "Seamless is not supported. Loading songs in directly") | ||
|
|
||
| if (plr.shuffleModeEnabled && mq.shuffleOrder == null) | ||
| Log.w(TAG, "Shuffle mode is enabled but no shuffle order is provided") |
There was a problem hiding this comment.
what about not storing that boolean and just making null mean shuffle is off? avoids inconsistency altogether. (shuffle order doesn't need to be stored if shuffle is off because enabling will reshuffle anyway)
Adds "basic" support for multiple queues. Other needed features (ex. lastplayedmanger, queue menu) will be added later, or much later in terms of nice to haves (ex. multiselect, search, etc).
To test: Enable in Flags.kt, then enable settings toggle